@llvm/pr-subscribers-llvm-binary-utilities

Author: Vitaly Buka (vitalybuka)

diff --git a/llvm/tools/llvm-size/llvm-size.cpp b/llvm/tools/llvm-size/llvm-size.cpp
index 0d7bf24832670..045237f161322 100644
--- a/llvm/tools/llvm-size/llvm-size.cpp
+++ b/llvm/tools/llvm-size/llvm-size.cpp
@@ -899,8 +899,10 @@ int llvm_size_main(int argc, char **argv, const llvm::ToolContext &) {
     OutputFormat = darwin;
   else if (V == "sysv")
     OutputFormat = sysv;
-  else
+  else {
     error("--format value should be one of: 'berkeley', 'darwin', 'sysv'");
+    return 1;
+  }
   V = Args.getLastArgValue(OPT_radix_EQ, "10");
   if (V == "8")
     Radix = RadixTy::octal;
@@ -908,8 +910,10 @@ int llvm_size_main(int argc, char **argv, const llvm::ToolContext &) {
     Radix = RadixTy::decimal;
   else if (V == "16")
     Radix = RadixTy::hexadecimal;
-  else
+  else {
     error("--radix value should be one of: 8, 10, 16 ");
+    return 1;
+  }
 
   for (const auto *A : Args.filtered(OPT_arch_EQ)) {
     SmallVector<StringRef, 2> Values;

A lot of utilities using LLVMOption, including clang and lld, are able to report multiple unknown option errors or other parsing errors before exiting. What the motivation behind this change?

What the motivation behind this change?

Bad printf format string, when radix is not one of enum.

If you realy think recovery is important, I'll change fallback to default on error.

What the motivation behind this change?

Bad printf format string, when radix is not one of enum.

If you realy think recovery is important, I'll change fallback to default on error.

Done

@vitalybuka if I understand @MaskRay advice correctly, we just want to finish the command line argument parsing before exiting because of errors.

The default value of OutputFormat is berkeley, so the assignment seems redundant.
I see that Radix does need a change. Perhaps initialize the global variable instead.

The default value of OutputFormat is berkeley, so the assignment seems redundant. I see that Radix does need a change. Perhaps initialize the global variable instead.

Done

The default value of OutputFormat is berkeley, so the assignment seems redundant. I see that Radix does need a change. Perhaps initialize the global variable instead.

Done

@thurstond @fmayer
I believe I made as @MaskRay suggested.
And the version of the patch does not change behavior.
So I guess either approval is fine to land.

Shouldn't we have a test case that shows the default value is correct? Presumably that test would have flagged the missing default initialization otherwise.

I thought that probably not, because it's about warning in Asan. It's not even report error, just log output, which is inconvenient noise for me.
Other that that the difference is in behavior outside of what this tool should promise. It's after the tool reported error and will exit with error code anyway.
We already tests that the tool fail with error, the rest of output is irrelevant.

However, if I read standard correctly, this case is UB https://godbolt.org/z/4G8xocceP
So it could be reasonable promise to have not UB on error, and then we should have a test.